Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extend install prompt functionality #2669

Merged

Conversation

IanButterworth
Copy link
Member

Adds two options and a help mode, while intending to not take up more room or overload with information.

Screen Shot 2021-07-11 at 10 59 06 PM

Screen Shot 2021-07-11 at 10 59 16 PM

Screen Shot 2021-07-11 at 10 59 36 PM

@StefanKarpinski @oxinabox would you mind giving this a go? The experience is a little hard to capture fully in screenshots.

Also, is there a better way to show the LOAD_PATH and a function to do the conversion?
i.e. instead of ["@", "@v#.#"] show ["/path/to/MyProject", "@v1.8"]

Note that I dropped @stdlib from the options

@KristofferC
Copy link
Member

Also, is there a better way to show the LOAD_PATH and a function to do the conversion?

There is Base.load_path_expand.(LOAD_PATH).

@fredrikekre
Copy link
Member

I think this is too complicated.

  • For newcomers (which I expect hit this error more often than others?) the concept of environments and load path are likely just confusing and in that case this PR is IMO causing more harm than good.
  • If you did using X you expected X to be available in the current load path, so why would you want to add it to a temp env? I honestly think this is super uncommon and when you need that you can do it yourself rather than building that functionality into this error handler.

src/REPLMode/REPLMode.jl Outdated Show resolved Hide resolved
@KristofferC
Copy link
Member

I think this is too complicated.

I must say I thought the same thing too. Especially the temp env thing.

@IanButterworth
Copy link
Member Author

Yeah over complication is a fair point. I definitely worried that as I put this together.

This might be a step too far into making a new duplicative repl mode

@oxinabox
Copy link
Contributor

oxinabox commented Jul 12, 2021

I must say I thought the same thing too. Especially the temp env thing.

I find myself doing this a lot
because I can't activate my test dependencies.
(unless i drop support for 1.0, and push a file on to the LOAD_PATH)
so I keep having to install them into a temp enviroment.

Maybe we should fix being able to activate test dependencies (without having to drop support for 1.0 instead).
I think I wouldn't have much use for this outside of that case.

@StefanKarpinski
Copy link
Member

I personally don't care about the temp environment bit (that was @oxinabox's request), but I really want the "other" option. It's really common to be working on a project and then do, say,using Revise, and if that's not in your global environment yet, then you want to install it there, rather than in the current active environment. The current workflow when that happens is:

  1. ] activate to deactivate the current project
  2. ] add Revise
  3. ] activate . to reactivate the current project, assuming you're in that directory

The temp thing could potentially be addressed folded into the other option by making o take you to a menu with more options, something like this:

julia> using JSON3
 │ Package JSON3 not found, but a package named JSON3 is available from a
 │ registry.
 │ Install package?
 │   (@v1.8) pkg> add JSON3
 └ (y/n/o) [y]: o

julia> using JSON3
 │ Package JSON3 not found, but a package named JSON3 is available from a registry.
 │ Install package?
 │   (Foo) pkg> add JSON3
 └ Select environment:
> 1: `~/dev/Foo` (@)
  2: `~/.julia/environments/v1.8` (@#.#)
  t: activate and install into new temporary environment

That way the first menu hardly changes and people who don't care about this aren't overwhelmed with options.

@StefanKarpinski
Copy link
Member

I find myself doing this a lot because I can't activate my test dependencies.

Ah, see, I just put these in my global shared environment. After a while I have the test and dev dependencies of everything I'm working on in there and it stops being a problem.

@IanButterworth
Copy link
Member Author

IanButterworth commented Jul 13, 2021

Updated to the last suggestion, although I couldn't work out how to make the prefix keybindings work in TerminalMenus

@IanButterworth
Copy link
Member Author

IanButterworth commented Jul 14, 2021

Updated with working keybindings for the o menu, depending on JuliaLang/julia#41576

julia> using JSON3
 │ Package JSON3 not found, but a package named JSON3 is available from a registry. 
 │ Install package?
 │   (Pkg) pkg> add JSON3 
 └ (y/n/o) [y]: o
julia> using JSON3
 │ Package JSON3 not found, but a package named JSON3 is available from a registry. 
 │ Install package?
 │   (Pkg) pkg> add JSON3 
 └ Select environment:
 > 1: `~/Documents/GitHub/Pkg.jl/Project.toml` (@)
   2: `~/.julia/environments/v1.8/Project.toml` (@v#.#)
   t: Activate and install into new temporary environment

For instance, pressing t will select the temp option

@StefanKarpinski
Copy link
Member

I'm getting this on Julia master:

ERROR: MethodError: no method matching REPL.TerminalMenus.Config(; keybindings=['1', '2', 't'])
Closest candidates are:
  REPL.TerminalMenus.Config(; charset, cursor, up_arrow, down_arrow, updown_arrow, scroll_wrap, ctrl_c_interrupt) at /Users/stefan/dev/julia/usr/share/julia/stdlib/v1.8/REPL/src/TerminalMenus/config.jl:45 got unsupported keyword argument "keybindings"
  REPL.TerminalMenus.Config(::Char, ::Char, ::Char, ::Char, ::Bool, ::Bool) at /Users/stefan/dev/julia/usr/share/julia/stdlib/v1.8/REPL/src/TerminalMenus/config.jl:6 got unsupported keyword argument "keybindings"
  REPL.TerminalMenus.Config(::Any, ::Any, ::Any, ::Any, ::Any, ::Any) at /Users/stefan/dev/julia/usr/share/julia/stdlib/v1.8/REPL/src/TerminalMenus/config.jl:6 got unsupported keyword argument "keybindings"

@IanButterworth
Copy link
Member Author

@StefanKarpinski

depending on JuliaLang/julia#41576

@StefanKarpinski
Copy link
Member

Ah, ok! Will try again on Monday but I'm fine with this being merged before then.

@IanButterworth
Copy link
Member Author

@fredrikekre @KristofferC what do you think about this simpler form?

I quite like it. I think there's also a nice side-effect of highlighting the env stacking in a way that's not visible elsewhere

@fredrikekre
Copy link
Member

@oxinabox: I find myself doing this a lot

Can you describe your workflow? Is it something like this:

  1. using BenchmarkTools # not in current env -> error
  2. activate --temp
  3. add BenchmarkTools
  4. using BenchmarkTools
  5. using JSON # which is in current env -> error
  6. activate .
  7. using JSON
  8. Success?

and with this PR it will reduce to

  1. using BenchmarkTools # not in current env -> error
  2. Select new "install to temp"
  3. using BenchmarkTools
  4. using JSON # which is in current env -> error
  5. activate .
  6. using JSON
  7. Success?

Doesn't seem like much of an improvement to me.

@StefanKarpinski: I really want the "other" option. It's really common to be working on a project and then do, say,using Revise, and if that's not in your global environment yet, then you want to install it there, rather than in the current active environment. The current workflow when that happens is: [...]

I guess I tend to just install Revise to the current env instead, and just not commit the changes to the toml files. This also has the benefit that your project is "consistent" (everything you use is from the same Pkg resolve).


I am not super opposed to the select env to install option, but I think the temp thing does too much and messes with the users active project etc.

@StefanKarpinski
Copy link
Member

I guess I tend to just install Revise to the current env instead, and just not commit the changes to the toml files. This also has the benefit that your project is "consistent" (everything you use is from the same Pkg resolve).

This seems pretty annoying — I'm already annoyed that I need to edit the project file to modify the UUID of stdlibs, at least with normal packages I'm developing I don't have to do that. Now I have to keep remembering not to accidentally commit extra dev dependencies that I added to the project just to test it? I mean, I guess I could live with it, but I find my current approach of just adding all the dev & test tools to my shared global environment much preferable. Compatibility with the environment of the project being developed has never been an actual issue for me doing this. I know that in theory it could be if there was some common dependency that was incompatible, but in practice it just hasn't been.

Btw, having subprojects would address the "resolved together" issue and if we added subprojects to the load path then it would address that issue as well since we'd resolve the subprojects at the same as we resolve the main project and then load the tools from the subproject environment. I think it would be nice to have a way to have the dev and test subprojects of the current active environment in the load path after the main project. In that situation, the "other" option here could also give the option to add packages to subprojects (possibly whether they're in the load path or not).

@StefanKarpinski
Copy link
Member

StefanKarpinski commented Jul 17, 2021

Btw, just to compare, my preferred workflow with this PR would become this:

  1. using BenchmarkTools # not in current env -> error
  2. Press o for other; Press 2 to install into @#.#
  3. using JSON

So I'd say that's by far the shortest, least annoying workflow described here. And again: if you don't like this workflow, it only adds a /o to the prompt, which people are free to ignore, whereas for people who do like my workflow, it's a huge convenience to have this option.

@StefanKarpinski
Copy link
Member

I do think that part of your point is that creating a temporary environment and activating it is a bit weird — you have to switch back to your previous active environment to continue your work anyway. My option doesn't have that issue because it doesn't activate a different environment, it just installs into a different one, leaving the active environment the same. It seems like what one really wants for @oxinabox's workflow is to create a temporary environment, add the test dependency to it and then add it to the load path right after the active environment. But that's a lot to bake into a convenience thing like this. That's why I think it might be better for that workflow to combine something like direnv to make the load path include a temporary test/dev environment that's created when you cd into the directory and then use the o option to just install into it.

@IanButterworth
Copy link
Member Author

It'd be good to figure out what to include in this PR.
@oxinabox would this still be helpful for you if we dropped the t option off?

@oxinabox
Copy link
Contributor

So I think the behavour I actually want is not:
Create a temp enviroment and activate it, and add things.
It is: create a temp enviroment, put it in the LOAD_PATH after the standard global enviroment (if there is not one already in load path), and add things to it.

Which I agree is a pretty complex operation.
So I would be down with not doing this.
The motivation is kinda similar to Stefan's want to put things into an enviroment further up load path.
Except I want that enviroment to be temporary.
I will illustrate more full below:

re: @fredrikekre

Can you describe your workflow?

yeah sure.
It is generally not BenchmarkTools since i put that in a global environment.
It is something from my test environment which i can't instantiate (if we fixed that in a good way, my need for this feature would basically vanish entirely).

What I want to be able to do is copy and paste in the block of code that is at the top of my runtests.jl that loads all the packages, and also sets some constants etc.
So that i can then run just one test.
And example of such a block of code is

sing Base.Broadcast: broadcastable
using ChainRules
using ChainRulesCore
using ChainRulesTestUtils
using ChainRulesTestUtils: rand_tangent, _fdm
using Compat: hasproperty, only, cispi, eachcol
using FiniteDifferences
using FiniteDifferences: rand_tangent
using LinearAlgebra
using LinearAlgebra.BLAS
using LinearAlgebra: dot
using Random
using StaticArrays
using Statistics
using Test

ChainRulesTestUtils.enable_tangent_transform!(Thunk)
Random.seed!(1) # Set seed that all testsets should reset to.

Future

  1. Copy and paste that block of code from the top of my runtests.jl.
  2. Get prompted only for the ones not currently loaded (in this case it would be StaticArrays, FiniteDifferences, and ChainRulesTestUtils)
  3. press ot, ot, ot, and all 3 would be installed and the rest of the code run
  4. Copy and past in the particular test that i want to run to dig into.

Total 4 steps

Realistic Current

  1. Copy and paste that block of code from the top of my runtests.jl.
  2. Get an error about ChainRulesTestUtils not being in enviroment and asked if i want to add. Decline as I don't want a direct dep
  3. ]activate --temp
  4. ] add ChainRulesTestUtils
  5. Either: repeat 1,2, and 4 again for StaticArrays and FiniteDifferences. Or spend time thinking about what test vs direct dependencies I have and add just those. (The former is 6 steps, the latter is about 3 steps of looking, thinking adding)
  6. Copy and paste that block of code from the top of my runtests.jl.
  7. Copy and past in the particular test that i want to run to dig into.

Total 9-12 steps

Ideal Current if I never made mistakes

  1. do ] activate --temp
  2. do ] add StaticArrays FiniteDifferences ChainRulesTestUtils
  3. do using StaticArrays, FiniteDifferences, ChainRulesTestUtils
  4. do activate .
  5. Copy and paste that block of code from the top of my runtests.jl.
  6. Copy and past in the particular test that i want to run to dig into.

Total 6 steps


The thing is not to compare the future again the version where I never make mistakes.
Since if I never made mistakes the original feature would also be much less useful since I would simiularly have all my main deps added.
But against the Realistic Current where I do make mistakes

@IanButterworth
Copy link
Member Author

On slack, @StefanKarpinski made the nice suggestion of adding insert!(LOAD_PATH, 2, mktempdir()) to startup.jl and removing the t option here, which @oxinabox and I thought was a nice solution.

So the t option is now removed

@IanButterworth IanButterworth merged commit 39cc95a into JuliaLang:master Jul 21, 2021
@IanButterworth IanButterworth deleted the ib/pkg_install_prompt_extended branch July 21, 2021 21:37
@IanButterworth IanButterworth added the backport 1.7 Change should be backported to release-1.7 label Jul 21, 2021
@IanButterworth
Copy link
Member Author

Marking this for 1.7 backport. Hopefully that's ok as it's a tweak to an already added 1.7 new feature

@StefanKarpinski
Copy link
Member

This is an additional feature so I don't think it should be backported. It's fine for 1.7 to include the y/n options only and for 1.8 to add the y/n/o option.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants